Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replica selector #692

Merged
merged 9 commits into from
Dec 13, 2024
Merged

feat: replica selector #692

merged 9 commits into from
Dec 13, 2024

Conversation

proost
Copy link
Contributor

@proost proost commented Dec 11, 2024

discussion: #688

When user set "SendToReplicas" only, set ReaderNodeSelector to replicaOnlySelector. Because It is same selecting replicas as reader node.

Currently, only adding reader node selector. AZ affinity or Low latency load balancing adding later.

cluster.go Outdated
return nil, ErrReplicaOnlyConflictWithReaderNodeSelector
}
if opt.SendToReplicas != nil && opt.ReaderNodeSelector != nil {
return nil, ErrSendToReplicasConflictWithReaderNodeSelector
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that we should not hide SendToReplicas when setting ReaderNodeSelector. That looks too implicit. Actually, I think we should ask users to provide SendToReplicas when ReaderNodeSelector is set:

Instead of simply (ReaderNodeSelector alone is hard for me to reason what it actually does):

	client, err := rueidis.NewClient(rueidis.ClientOption{
		ReaderNodeSelector: func(slot uint16, replicas []rueidis.ReplicaInfo) int {
			return rand.IntN(len(replicas))
		},
	})

I prefer making it more explicitly state:

	client, err := rueidis.NewClient(rueidis.ClientOption{
		SendToReplicas: func(cmd rueidis.Completed) bool {
			return cmd.IsReadOnly()
		},
		ReplicaSelector: func(slot uint16, replicas []rueidis.ReplicaInfo) int {
			return rand.IntN(len(replicas))
		},
	})

I believe the latter provides users with a better sense of control over which replicas to execute which commands.

Also, I think changing the name of ReaderNodeSelector to ReplicaSelector is more consistent with the existing naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Using SendToReplicas and ReplicaSelector both makes sense to me. Even though user can select primary using ReplicaSelector, It is configured explicitly, so it is clear to me.
90387d2

@proost proost changed the title feat: reader node selector feat: replica selector Dec 12, 2024
@proost proost requested a review from rueian December 12, 2024 15:24
cluster.go Outdated
Comment on lines 261 to 262
replicas := make([]ReplicaInfo, 0, n)
for _, addr := range g.nodes[1:] {
Copy link
Collaborator

@rueian rueian Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid this allocation by replacing g.nodes from []string to []ReplicaInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already consider it. But i didn't. Because

  1. g.nodes has primary node too. so ReplicaInfo slice type is incorrect. It can be misleading to contributors.
  2. user can change value each ReplicaInfo. Changing ReplicaInfo is not problem to rueidis. but there can be side effect in the user side.

Because of duplicated allocation, i also considering reusing ReplicaInfo slice. But problem is If user holds ReplicaInfo slice in the ReplicaSelector it can be problem.

I prefer reusing ReplicaInfo slice if should reduce allocation. And in this case, we should add comment that user must not store reference ReplicaInfo slice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @proost,

Great considerations!

  1. If naming is the concern, we can use some sort of the type alias. We can only expose ReplicaInfo to users but use another name internally.
  2. We can add comments on the selector function to tell the user should not change values in the slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

50fad71

OK, I changed it.

@proost proost requested a review from rueian December 13, 2024 15:32
@rueian rueian merged commit 29524d2 into redis:main Dec 13, 2024
30 checks passed
@rueian
Copy link
Collaborator

rueian commented Dec 13, 2024

LGTM. Thanks @proost!

@proost proost deleted the feat-reader-node-selector branch December 15, 2024 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants